Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #396 +/- ##
==========================================
+ Coverage 85.13% 85.30% +0.17%
==========================================
Files 53 53
Lines 5784 5941 +157
Branches 649 652 +3
==========================================
+ Hits 4924 5068 +144
- Misses 849 862 +13
Partials 11 11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cppcheck (reported by Codacy) found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
There was a problem hiding this comment.
Pull request overview
Adds a RoadStatus concept (OPEN/CLOSED) to streets and integrates it into routing so pathfinding can ignore closed roads.
Changes:
- Introduces
dsf::mobility::RoadStatusand stores per-road status inRoad. - Adds
RoadNetwork::setStreetStatusById/ByNameand updatesallPathsTo/shortestPathto skip CLOSED roads. - Extends C++ tests and exposes
RoadStatus+ street status/capacity APIs via pybind11.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/mobility/Test_graph.cpp | Adds test coverage for status toggling and routing behavior with closed roads. |
| src/dsf/mobility/RoadNetwork.hpp | Declares status setters and updates Dijkstra-based routing to ignore CLOSED streets. |
| src/dsf/mobility/RoadNetwork.cpp | Implements status setters (by id and by name/substring). |
| src/dsf/mobility/Road.hpp | Adds RoadStatus enum and stores default OPEN status on roads. |
| src/dsf/bindings.cpp | Binds RoadStatus and exposes status/capacity mutation methods to Python. |
Comments suppressed due to low confidence (2)
src/dsf/mobility/RoadNetwork.hpp:390
- In
allPathsTo,edge(inEdgeId)is now looked up multiple times per iteration (status check,source(), and passing tof(...)). Since this is an unordered_map lookup, consider storingauto const& e = edge(inEdgeId);once and reusing it to avoid repeated lookups and keep the loop easier to read.
// Skip closed roads
if (edge(inEdgeId)->roadStatus() == RoadStatus::CLOSED) {
continue;
}
Id neighborId = edge(inEdgeId)->source();
// Calculate the weight of the edge from neighbor to currentNode using the dynamics function
double edgeWeight = f(this->edge(inEdgeId));
src/dsf/mobility/RoadNetwork.hpp:500
- In
shortestPath,edge(inEdgeId)is now looked up multiple times per iteration (status check,source(), and passing tof(...)). Consider caching the pointer/reference for the current edge inside the loop to avoid repeated unordered_map lookups and improve readability.
// Skip closed roads
if (edge(inEdgeId)->roadStatus() == RoadStatus::CLOSED) {
continue;
}
Id neighborId = edge(inEdgeId)->source();
// Calculate the weight of the edge from neighbor to currentNode using the dynamics function
double edgeWeight = f(this->edge(inEdgeId));
double newDistToTarget = distToTarget[currentNode] + edgeWeight;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| /// @brief Set the street's status by its id | ||
| /// @param streetId The id of the street | ||
| /// @param status The status to set |
There was a problem hiding this comment.
setStreetStatusById calls edge(streetId) which uses unordered_map::at and will throw std::out_of_range if the id doesn’t exist (see src/dsf/base/Network.hpp:224-230). Consider either documenting this in the comment or catching it and throwing a more specific exception (e.g., std::invalid_argument) consistent with other public RoadNetwork APIs.
| /// @param status The status to set | |
| /// @param status The status to set | |
| /// @throws std::out_of_range if the given street id does not exist in the network |
1 participant
Pull request overview
Adds a
RoadStatusconcept (OPEN/CLOSED) to streets and integrates it into routing so pathfinding can ignore closed roads.Changes:
dsf::mobility::RoadStatusand stores per-road status inRoad.RoadNetwork::setStreetStatusById/ByNameand updatesallPathsTo/shortestPathto skip CLOSED roads.RoadStatus+ street status/capacity APIs via pybind11.Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
RoadStatusenum and stores default OPEN status on roads.RoadStatusand exposes status/capacity mutation methods to Python.Comments suppressed due to low confidence (2)
src/dsf/mobility/RoadNetwork.hpp:390
allPathsTo,edge(inEdgeId)is now looked up multiple times per iteration (status check,source(), and passing tof(...)). Since this is an unordered_map lookup, consider storingauto const& e = edge(inEdgeId);once and reusing it to avoid repeated lookups and keep the loop easier to read.src/dsf/mobility/RoadNetwork.hpp:500
shortestPath,edge(inEdgeId)is now looked up multiple times per iteration (status check,source(), and passing tof(...)). Consider caching the pointer/reference for the current edge inside the loop to avoid repeated unordered_map lookups and improve readability.💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.